Allow specific version to be deleted from cache #4

yrumax · 2022-05-24T13:31:03Z

You can now specify a specific ref to be removed from the cache
rather than having to remove all of them

Restores the nested cache structure from before SECURITY-2586.

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

yrumax · 2022-05-24T13:32:13Z

@jglick I have moved my pull request over to this repository for review as requested.

jglick · 2022-05-24T14:07:38Z

jenkinsci/workflow-cps-global-lib-plugin#142 for the record.

yrumax · 2022-05-24T14:30:02Z

I'm working with this plugin the same as workflow-cps-global-lib-plugin but all the tests locally fail with an IllegalStateException on this line. Am I missing something new with this project setup?

jglick · 2022-05-24T14:36:51Z

If an @Extension seems to be missing at runtime, try

mvn -Pquick-build clean install

(Ultimately probably a bug in either the annotation processor or the Maven compiler plugin or somehow the interaction between the two, I just have never tried to track it down.)

yrumax · 2022-05-24T14:52:49Z

Cool. That did the trick. After running your command the tests are working again. Seems like it's probably some specific eclipse m2e interaction that's failing.

I'll investigate those failing tests. They probably don't like how I restored the nested cache directory structure.

yrumax · 2022-05-24T16:13:12Z

Unfortunately I only have a windows machine to test on and I can't run that symlink test locally. I looked over the logic that it's calling and I don't have much of an explanation as to why that test fails. LibraryRecord.getDirectoryName() returns the new nested path and that's used to compare LibraryAdder.findResources. I'll load my fork on my linux build server later today and try to debug further.

yrumax · 2022-05-25T15:34:10Z

The getConnicalFile method does not appear to be returning the symlink path as expected.
//LOGGER.severe("path: " + new File(f.getRemote()).getCanonicalFile().toPath());

2.810 [id=51] SEVERE o.j.p.workflow.libs.LibraryAdder#findResources: path: /home/jenkins/workspace/_pipeline-groovy-lib-plugin_PR-4/target/tmp/j h1113867621410959948/jobs/p/builds/1/libs/5fbd776874e55a452cf050b3df731fe13a36dbcebf52977cb67b1fef6615d8f3/d130612e7c729d275f2281426a605830ebce666d4089ff4ec5ded2322d6e773d/resources/master.key
2.810 [id=51] SEVERE o.j.p.workflow.libs.LibraryAdder#findResources: library path: /home/jenkins/workspace/_pipeline-groovy-lib-plugin_PR-4/target/tmp/j h1113867621410959948/jobs/p/builds/1/libs/5fbd776874e55a452cf050b3df731fe13a36dbcebf52977cb67b1fef6615d8f3/d130612e7c729d275f2281426a605830ebce666d4089ff4ec5ded2322d6e773d/resources

I will continue to investigate.

yrumax · 2022-05-25T16:37:59Z

@jglick I think the root cause of the failing test is that getCanonicalFile only works on symlinks that actually exist. secrets/master.key an actual file (whoops). When I updated the library directory path to include an additional folder I broke the symlink.

I will clean up my commits and then this pull request will be ready for review. Is there a style guide for ordering imports? I see that eclipse rearranged them. I'll reorder them.

jglick · 2022-05-25T16:45:18Z

To be clear, I am not claiming to maintain this plugin generally (I needed to do the split from workflow-cps-global-lib for architectural reasons unrelated to logical content) so any such comments should be addressed to the actual maintainers, if any.

yrumax · 2022-05-25T16:46:49Z

Who should I loop in to the pr via comments? It seems like the default permissions don't allow me to add reviewers.

mawinter69 · 2022-05-26T07:14:12Z

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration.java

+                                if (libraryCachePath != null) {
+                                    FilePath versionCachePath = new FilePath(libraryCachePath, libraryNamePath.getName().replace("-name.txt", ""));
+                                    if (cachedLibraryRef != null && !cachedLibraryRef.equals("")) {
+                                        if (libraryNamePath.readToString().equals(name + "@" + cachedLibraryRef)) {


use StringUtils.isNotBlank(cachedLibraryRef) to also catch strings containing only whitespace

mawinter69 · 2022-05-26T07:19:28Z

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration.java

+                                        }
+                                    } else {
+                                        cacheDirName = name;
+                                        libraryCachePath.deleteRecursive();


When #3 is merged you will not be able to delete like this. You must delete each version separately to ensure it is not just getting used by any other operation (e.g. cache was expired and is getting refreshed, a build is just copying it).

Sure. I will integrate your changes depending on which gets merged first.

yrumax · 2022-08-04T16:59:33Z

@mawinter69 as somewhat of an initial compromise with simplicity in mind I have chosen to lock the entire library folder while performing read or write actions. This will force a wait for library@version1 and library@version2 because the lock being acquired is "library". I am not opposed to individually locking each sub directory however cleanup of the entire set of caches becomes much more complicated.

yrumax · 2022-08-04T17:00:36Z

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryAdder.java

@@ -202,7 +202,7 @@ static List<URL> retrieve(@NonNull LibraryRecord record, @NonNull LibraryRetriev
        FilePath libDir = new FilePath(execution.getOwner().getRootDir()).child("libs/" + record.getDirectoryName());
        Boolean shouldCache = cachingConfiguration != null;
        final FilePath versionCacheDir = new FilePath(LibraryCachingConfiguration.getGlobalLibrariesCacheDir(), record.getDirectoryName());
-        ReentrantReadWriteLock retrieveLock = getReadWriteLockFor(record.getDirectoryName());
+        ReentrantReadWriteLock retrieveLock = getReadWriteLockFor(record.getName());


This is to make it consistent across accesses because record.getName() is no longer equal to record.getDirectoryName().

You can now specify a specific ref to be removed from the cache rather than having to remove all of them

yrumax · 2022-10-17T13:24:43Z

@jglick @car-roll @mawinter69 Any feedback on this pull request?

yrumax force-pushed the feature/target-cache-refresh branch from 0d00e64 to e384a3f Compare May 24, 2022 16:07

yrumax force-pushed the feature/target-cache-refresh branch from defd8cd to 5444f8b Compare May 25, 2022 15:16

yrumax force-pushed the feature/target-cache-refresh branch from 5444f8b to 335fe2d Compare May 25, 2022 15:42

yrumax force-pushed the feature/target-cache-refresh branch from ada6bad to 877434b Compare May 25, 2022 16:57

mawinter69 reviewed May 26, 2022

View reviewed changes

yrumax force-pushed the feature/target-cache-refresh branch from 877434b to 4f59a0b Compare May 26, 2022 17:59

yrumax force-pushed the feature/target-cache-refresh branch from 4f59a0b to f261a36 Compare August 4, 2022 16:53

yrumax commented Aug 4, 2022

View reviewed changes

yrumax force-pushed the feature/target-cache-refresh branch 2 times, most recently from 1f44c2d to cee52c0 Compare August 4, 2022 17:14

Allow specific version to be deleted from cache

a9f1645

You can now specify a specific ref to be removed from the cache rather than having to remove all of them

yrumax force-pushed the feature/target-cache-refresh branch from cee52c0 to a9f1645 Compare August 5, 2022 15:02

AnishaGharat mentioned this pull request Aug 16, 2022

[JENKINS-69135] Add a "Versions to include" field to the Global Library Cache feature #16

Merged

6 tasks

Merge branch 'jenkinsci:master' into feature/target-cache-refresh

71dc7c2

yrumax requested a review from a team as a code owner January 20, 2023 15:55

Allow specific version to be deleted from cache #4

Are you sure you want to change the base?

Allow specific version to be deleted from cache #4

Uh oh!

Conversation

yrumax commented May 24, 2022

Uh oh!

yrumax commented May 24, 2022

Uh oh!

jglick commented May 24, 2022

Uh oh!

yrumax commented May 24, 2022

Uh oh!

jglick commented May 24, 2022

Uh oh!

yrumax commented May 24, 2022

Uh oh!

yrumax commented May 24, 2022

Uh oh!

yrumax commented May 25, 2022

Uh oh!

yrumax commented May 25, 2022

Uh oh!

jglick commented May 25, 2022

Uh oh!

yrumax commented May 25, 2022

Uh oh!

mawinter69 May 26, 2022

Choose a reason for hiding this comment

Uh oh!

mawinter69 May 26, 2022

Choose a reason for hiding this comment

Uh oh!

yrumax May 26, 2022

Choose a reason for hiding this comment

Uh oh!

yrumax commented Aug 4, 2022

Uh oh!

yrumax Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

yrumax commented Oct 17, 2022

Uh oh!

Uh oh!